Some Suggested Improvements for Text Tool#4355
Open
thermalPasteMilkshake wants to merge 7 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 2/5
- In
editor/src/messages/tool/tool_messages/select_tool.rs, the Deepest-selection edit handler is replaced with an empty block, which appears to disable double-click-to-edit for text/path layers and leavesedit_layer_deepest_manipulationeffectively unused; merging as-is likely causes a clear editing workflow regression in the default selection mode — restore or rewire the Deepest edit path (and verify with a focused interaction test) before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/tool_messages/select_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/select_tool.rs:1129">
P1: Replacing the Deepest-selection edit handler with an empty block silently disables double-click-to-edit for text/path layers in the default 'Deepest' selection mode, and leaves `edit_layer_deepest_manipulation` as dead code. This looks like leftover debugging rather than an intentional fix and should be reverted or reworked.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| match tool_data.nested_selection_behavior { | ||
| NestedSelectionBehavior::Shallowest => edit_layer_shallowest_manipulation(document, intersect, responses), | ||
| NestedSelectionBehavior::Deepest => edit_layer_deepest_manipulation(intersect, &document.network_interface, responses), | ||
| NestedSelectionBehavior::Deepest => {} |
Contributor
There was a problem hiding this comment.
P1: Replacing the Deepest-selection edit handler with an empty block silently disables double-click-to-edit for text/path layers in the default 'Deepest' selection mode, and leaves edit_layer_deepest_manipulation as dead code. This looks like leftover debugging rather than an intentional fix and should be reverted or reworked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/select_tool.rs, line 1129:
<comment>Replacing the Deepest-selection edit handler with an empty block silently disables double-click-to-edit for text/path layers in the default 'Deepest' selection mode, and leaves `edit_layer_deepest_manipulation` as dead code. This looks like leftover debugging rather than an intentional fix and should be reverted or reworked.</comment>
<file context>
@@ -1125,7 +1126,7 @@ impl Fsm for SelectToolFsmState {
match tool_data.nested_selection_behavior {
NestedSelectionBehavior::Shallowest => edit_layer_shallowest_manipulation(document, intersect, responses),
- NestedSelectionBehavior::Deepest => edit_layer_deepest_manipulation(intersect, &document.network_interface, responses),
+ NestedSelectionBehavior::Deepest => {}
}
}
</file context>
Suggested change
| NestedSelectionBehavior::Deepest => {} | |
| NestedSelectionBehavior::Deepest => edit_layer_deepest_manipulation(intersect, &document.network_interface, responses), |
Author
|
update:
Also AI Disclosure: I used it to understand code and identify pain points, also to write parts of it but those parts are now deleted as they were for an entirely wrong issue but now the fix is back in the correct direction. |
thermalPasteMilkshake
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 3/5
- In
editor/src/messages/tool/tool_messages/text_tool.rs, hit testing uses the layer’s rendered/graph text bounds while edit mode displays content fromtool_data.new_text, so clicks can miss the actual visible text area and cause unreliable selection/edit interactions; align hit testing with the live edit buffer bounds during editing to de-risk regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/tool_messages/text_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/text_tool.rs:488">
P2: Clicks are tested against the layer's rendered/graph text bounds even though editing hides that text and the live content is stored in `tool_data.new_text`. The hit test therefore does not reliably cover the visible editable textbox, so clicking within the editor can incorrectly trigger the outside-click path. The bounds should be derived from the active editing text and its current `new_text` (using the same coordinate transform as the overlay).</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
|
|
||
| let quad = | ||
| document.metadata().transform_to_viewport(self.layer) | ||
| * text_bounding_box(self.layer, document, fonts, responses); |
Contributor
There was a problem hiding this comment.
P2: Clicks are tested against the layer's rendered/graph text bounds even though editing hides that text and the live content is stored in tool_data.new_text. The hit test therefore does not reliably cover the visible editable textbox, so clicking within the editor can incorrectly trigger the outside-click path. The bounds should be derived from the active editing text and its current new_text (using the same coordinate transform as the overlay).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/text_tool.rs, line 488:
<comment>Clicks are tested against the layer's rendered/graph text bounds even though editing hides that text and the live content is stored in `tool_data.new_text`. The hit test therefore does not reliably cover the visible editable textbox, so clicking within the editor can incorrectly trigger the outside-click path. The bounds should be derived from the active editing text and its current `new_text` (using the same coordinate transform as the overlay).</comment>
<file context>
@@ -471,6 +471,27 @@ struct TextToolData {
+
+ let quad =
+ document.metadata().transform_to_viewport(self.layer)
+ * text_bounding_box(self.layer, document, fonts, responses);
+
+ !quad.contains(mouse)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello, This fixes #4328
(this is just a rough fix, I just wanted to ask some stuff out alongside it)
Im also requesting guidance on the outer border removal as I stated in the discussions for the issue.